Create app context switch test helper #3371
Conversation
- Added a test helper to get/set app context switch values. - Updated existing tests to use the helper. - Added missing switches to LocalAppContextSwitches tests.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new test helper—LocalAppContextSwitchesHelper—to safely capture and restore global app context switches using the RAII pattern. The changes update various test projects to utilize the helper instead of direct reflection-based switch manipulation, refactor the TdsParserStateObject to dispose of switch state correctly, and adjust some test inline data to match the new API.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs | Replaced direct switch handling with the helper to ensure proper restoration. |
| tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj | Added compilation reference for the new helper. |
| tests/FunctionalTests/TdsParserStateObject.TestHarness.cs | Refactored to use a disposable helper instance and implemented IDisposable. |
| tests/FunctionalTests/SqlParameterTest.cs | Updated test inline data and usage of the helper for switch configuration. |
| tests/FunctionalTests/MultiplexerTests.cs | Converted state object instantiation to a using statement for proper disposal. |
| tests/FunctionalTests/Microsoft.Data.SqlClient.Tests.csproj | Included the helper compilation. |
| tests/FunctionalTests/LocalAppContextSwitchesTests.cs | Amended inline test data to cover new switch properties. |
| tests/Common/LocalAppContextSwitchesHelper.cs | Introduced the new helper class implementation. |
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/tests/FunctionalTests/TdsParserStateObject.TestHarness.cs:161
- [nitpick] Consider renaming the 'LocalAppContextSwitches' field to clearly indicate it is an instance of the helper rather than the original static switch holder.
private SwitchesHelper LocalAppContextSwitches = new();
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlParameterTest.cs:1947
- The removal of test cases using null switch values changes the expected behavior. Please verify that omitting these cases is intentional and that the documentation and coverage adequately address the default switch behavior.
[InlineData(null, 7, null)]
paulmedynski
added
the
Area\Tests
| @@ -1945,89 +1947,29 @@ private enum Int64Enum : long | |||
| [InlineData(5, 5, false)] | |||
| [InlineData(6, 6, false)] | |||
| [InlineData(7, 7, false)] | |||
| [InlineData(null, 7, null)] | |||
There was a problem hiding this comment.
These data sets were just testing that the LegacyVarTimeZeroScale switch defaults to True, which is now covered above.
| @@ -17,7 +19,7 @@ internal struct PacketHandle | |||
| { | |||
| } | |||
| #endif | |||
| internal partial class TdsParserStateObject | |||
| internal partial class TdsParserStateObject : IDisposable | |||
There was a problem hiding this comment.
This harness adds implementation to TdsParserStateObject, and then the test project explicitly includes some of the MDS source files so they get compiled together. I think we need to revisit this approach.
For now, I have made this implementation disposable and updated the multiplexer tests to take advantage of that.
| get | ||
| { | ||
| var switchesType = typeof(SqlCommand).Assembly.GetType("Microsoft.Data.SqlClient.LocalAppContextSwitches"); | ||
| private SwitchesHelper LocalAppContextSwitches = new(); |
There was a problem hiding this comment.
This member needs to match the MDS internal class name that the MDS portion of TdsParserStateObject expects. The real MDS code resolves this as a class type (LocalAppContextSwitches), whereas the test resolves it as a the private field whose type is LocalAppContextSwitchesHelper - yikes!
- Fixed net462 compile error.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3371 +/- ##
==========================================
- Coverage 67.04% 64.34% -2.70%
==========================================
Files 300 300
Lines 65376 65376
==========================================
- Hits 43831 42069 -1762
- Misses 21545 23307 +1762
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This change adds a test helper to get/set app context switch values. The
LocalAppContextSwitchesclass is internal and global, which makes it difficult to manipulate safely in the tests. The helper implements the RAII pattern, capturing the switch values on construction and restoring them on disposal. The helper also provides reflection-based access to the properties and fields ofLocalAppContextSwitchesso tests can manufacture the environment they need.The new
LocalAppContextSwitchesHelperclass is needed by both the Functional and Manual tests, so I put it into a new siblingCommondirectory and reference it directly in the two test projects.All existing tests that touched the app context switches have been updated.
Issues
Fixes #3370 .
Testing
Modified tests were run locally and are passing. CI will run full regression.